reclaimspace: fix schedule handling #604

iPraveenParihar · 2024-06-26T07:34:52Z

In my earlier PR #585, I missed few changes 😞

This commit addresses a bug in determineScheduleAndRequeue where the check for schedule from the StorageClass annotations is never reached.

Madhu-1 · 2024-06-26T07:51:02Z

@iPraveenParihar can you please add a unit testing to cover a few test cases for PVC and SC and not found case for namespace for now?

iPraveenParihar · 2024-06-27T05:43:51Z

@iPraveenParihar can you please add a unit testing to cover a few test cases for PVC and SC and not found case for namespace for now?

@Madhu-1 added!

Madhu-1 · 2024-06-27T08:13:41Z

controllers/csiaddons/persistentvolumeclaim_controller.go

+			// The request needs to be requeued for checking
+			// driver support again.
+			return "", ErrConnNotFoundRequeueNeeded
+		}


I was thinking about the same where storageclass is deleted after the PVC creation, we need to consider not found error as the scheduleNotFound so as not to fail the reconcile because it's possible.

well IMO, we should be returning the precise error so that the users are aware of the exact state of the resources and they get it resolved?

it would have made sense it was a VR controller but this is a pvc controller and user just created the PVC object nothing else. can you please check it again and let me know if am missing something.

it would have made sense it was a VR controller but this is a pvc controller and user just created the PVC object nothing else. can you please check it again and let me know if am missing something.

@Madhu-1, Apologies, but I quite didn't understand this part.
I understand your concern about reconcile failures when StorageClass is not found. However, masking the SC notFound error with scheduleNotFound error will lead to confusion? Its users responsibility to take appropriate actions when SC notFound error is thrown?

Or did I miss your point?

take the case of what happens below

PVC is created

Storageclass is deleted

The reconciliation triggered for the PVC as we get the pvc and check for annotation, if the SC is not found we keep on reqeueing this PVC which never completes, isn't it?

Hmm, okay. I was talking about the same scenario. In such cases, there are two ways to handle it:

Return a "StorageClass not found" error, and the reconciliation process continues to requeue the PVC. This allows the user to take appropriate action on the error.

Return a "ScheduleNotFound" error and exit the reconciliation process. This approach masks the "StorageClass not found" error, potentially causing confusion for the user.

If masking the error isn't a concern, we can agree on returning the ScheduleNotFound error?

as both are independent objects once created, masking the error makes sense to me and avoids a lot of reconciles, @nixpanic @Rakshith-R WDYT?

I would prefer if we just log the sc not found info and move on.

Pull request has been modified.

iPraveenParihar · 2024-07-02T08:59:58Z

@Madhu-1 @Rakshith-R PTAL.

This commit addresses a bug in `determineScheduleAndRequeue` where the check for schedule from the StorageClass annotations is never reached. Signed-off-by: Praveen M <[email protected]>

Signed-off-by: Praveen M <[email protected]>

iPraveenParihar requested a review from Rakshith-R June 26, 2024 07:34

iPraveenParihar self-assigned this Jun 26, 2024

iPraveenParihar marked this pull request as ready for review June 26, 2024 07:39

mergify bot requested review from karthik-us, nixpanic, riya-singhal31 and yati1998 June 26, 2024 07:39

iPraveenParihar force-pushed the reclaimspace/sc-annotation-automate branch from 5079786 to dcac38b Compare June 27, 2024 05:44

Rakshith-R previously approved these changes Jun 27, 2024

View reviewed changes

Madhu-1 reviewed Jun 27, 2024

View reviewed changes

iPraveenParihar requested a review from Madhu-1 July 1, 2024 11:07

iPraveenParihar force-pushed the reclaimspace/sc-annotation-automate branch from dcac38b to 5387a51 Compare July 2, 2024 08:38

iPraveenParihar requested a review from Rakshith-R July 2, 2024 08:38

Rakshith-R approved these changes Jul 2, 2024

View reviewed changes

Madhu-1 approved these changes Jul 2, 2024

View reviewed changes

iPraveenParihar added 2 commits July 2, 2024 09:24

reclaimspace: fix schedule handling

6971dd9

This commit addresses a bug in `determineScheduleAndRequeue` where the check for schedule from the StorageClass annotations is never reached. Signed-off-by: Praveen M <[email protected]>

reclaimspace: add unit test case for schedule handling

ef5c97d

Signed-off-by: Praveen M <[email protected]>

iPraveenParihar force-pushed the reclaimspace/sc-annotation-automate branch from 5387a51 to ef5c97d Compare July 2, 2024 09:24

mergify bot merged commit 542ecb8 into csi-addons:main Jul 2, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reclaimspace: fix schedule handling #604

reclaimspace: fix schedule handling #604

iPraveenParihar commented Jun 26, 2024

Madhu-1 commented Jun 26, 2024

iPraveenParihar commented Jun 27, 2024

Madhu-1 Jun 27, 2024

iPraveenParihar Jun 27, 2024

Madhu-1 Jun 27, 2024

iPraveenParihar Jun 27, 2024 •

edited

Loading

Madhu-1 Jul 1, 2024 •

edited

Loading

iPraveenParihar Jul 1, 2024

Madhu-1 Jul 1, 2024

Rakshith-R Jul 2, 2024

iPraveenParihar commented Jul 2, 2024

reclaimspace: fix schedule handling #604

reclaimspace: fix schedule handling #604

Conversation

iPraveenParihar commented Jun 26, 2024

Madhu-1 commented Jun 26, 2024

iPraveenParihar commented Jun 27, 2024

Madhu-1 Jun 27, 2024

Choose a reason for hiding this comment

iPraveenParihar Jun 27, 2024

Choose a reason for hiding this comment

Madhu-1 Jun 27, 2024

Choose a reason for hiding this comment

iPraveenParihar Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Madhu-1 Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

iPraveenParihar Jul 1, 2024

Choose a reason for hiding this comment

Madhu-1 Jul 1, 2024

Choose a reason for hiding this comment

Rakshith-R Jul 2, 2024

Choose a reason for hiding this comment

iPraveenParihar commented Jul 2, 2024

iPraveenParihar Jun 27, 2024 •

edited

Loading

Madhu-1 Jul 1, 2024 •

edited

Loading